tools: fix skip detection of test runner output #53545

richardlau · 2024-06-22T03:56:35Z

Fix the Python test harness so that it no longer treats the # skipped part of the summary at the end of the built-in test runner output as marking the test as skipped.

Fixes: #50346

So it turns out that the Python test runner scans the output of the tests to look for SKIP directives:

node/tools/test.py

Line 86 in d335487

skip_regex = re.compile(r'# SKIP\S*\s+(.*)', re.IGNORECASE)

node/tools/test.py

Lines 388 to 391 in d335487

    
           skip = skip_regex.search(output.output.stdout) 
        
           if skip: 
        
             logger.info( 
        
               'ok %i %s # skip %s' % (self._done, command, skip.group(1)))

(this regex is based on http://testanything.org/tap-specification.html#skipping-tests).

For the tests that are using the built-in test runner, this is matching the # skipped line in the summary at the end of the output,
e.g.

$ ./node --test --test-reporter=tap test/parallel/test-dotenv-node-options.js
TAP version 13
# Subtest: .env supports NODE_OPTIONS
    # Subtest: should have access to permission scope
    ok 1 - should have access to permission scope
      ---
      duration_ms: 26.0744
      ...
    # Subtest: validate only read permissions are enabled
    ok 2 - validate only read permissions are enabled
      ---
      duration_ms: 22.168635
      ...
    # Subtest: TZ environment variable
    ok 3 - TZ environment variable
      ---
      duration_ms: 19.970299
      ...
    1..3
ok 1 - .env supports NODE_OPTIONS
  ---
  duration_ms: 69.387679
  type: 'suite'
  ...
1..1
# tests 3
# suites 1
# pass 3
# fail 0
# cancelled 0
# skipped 0
# todo 0
# duration_ms 144.850498
$

which causes the Python test harness to insert a SKIP directive with the number of skipped tests (e.g. 0 in this case) as the reason for skipping:

$ ./tools/test.py --progress=tap parallel/test-dotenv-node-options
TAP version 13
1..1
ok 1 parallel/test-dotenv-node-options # skip 0
  ---
  duration_ms: 213.30700
  ...

All tests passed.
$

With the updated regex:

$ ./tools/test.py --progress=tap parallel/test-dotenv-node-options
TAP version 13
1..1
ok 1 parallel/test-dotenv-node-options
  ---
  duration_ms: 212.83200
  ...

All tests passed.
$

cc @nodejs/test_runner

Fix the Python test harness so that it no longer treats the `# skipped` part of the summary at the end of the built-in test runner output as marking the test as skipped.

nodejs-github-bot · 2024-06-22T05:51:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59924/

aduh95 · 2024-06-22T06:23:46Z

tools/test.py

@@ -83,7 +83,7 @@ def get_module(name, path):


 logger = logging.getLogger('testrunner')
-skip_regex = re.compile(r'# SKIP\S*\s+(.*)', re.IGNORECASE)
+skip_regex = re.compile(r'(?:\d+\.\.\d+|ok|not ok).*# SKIP\S*\s+(.*)', re.IGNORECASE)


Wouldn't we want to not match line returns?

Suggested change

skip_regex = re.compile(r'(?:\d+\.\.\d+|ok|not ok).*# SKIP\S*\s+(.*)', re.IGNORECASE)

skip_regex = re.compile(r'(?:\d+\.\.\d+|ok|not ok)[^\n]*# SKIP\S*\s+(.*)', re.IGNORECASE)

The . character already excludes newlines.

https://docs.python.org/3/library/re.html

(Dot.) In the default mode, this matches any character except a newline. If the DOTALL flag has been specified, this matches any character including a newline.

nodejs-github-bot · 2024-06-24T12:09:32Z

Commit Queue failed

- Loading data for nodejs/node/pull/53545
✔  Done loading data for nodejs/node/pull/53545
----------------------------------- PR info ------------------------------------
Title      tools: fix skip detection of test runner output (#53545)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     richardlau:testharnessskip -> nodejs:main
Labels     test, tools
Commits    1
 - tools: fix skip detection of test runner output
Committers 1
 - Richard Lau 
PR-URL: https://github.com/nodejs/node/pull/53545
Fixes: https://github.com/nodejs/node/issues/50346
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Luigi Pinca 
Reviewed-By: Chemi Atlow 
Reviewed-By: Moshe Atlow 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/53545
Fixes: https://github.com/nodejs/node/issues/50346
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Luigi Pinca 
Reviewed-By: Chemi Atlow 
Reviewed-By: Moshe Atlow 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sat, 22 Jun 2024 03:56:35 GMT
   ✔  Approvals: 4
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/53545#pullrequestreview-2133822428
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/53545#pullrequestreview-2133854918
   ✔  - Chemi Atlow (@atlowChemi): https://github.com/nodejs/node/pull/53545#pullrequestreview-2133920028
   ✔  - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/53545#pullrequestreview-2133923840
   ✘  Last GitHub CI failed
   ℹ  Green GitHub CI is sufficient
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/9645012534

nodejs-github-bot · 2024-06-24T13:05:59Z

Landed in 2068c40

Fix the Python test harness so that it no longer treats the `# skipped` part of the summary at the end of the built-in test runner output as marking the test as skipped. PR-URL: #53545 Fixes: #50346 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

nodejs-github-bot added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Jun 22, 2024

richardlau mentioned this pull request Jun 22, 2024

parallel/test-dotenv-node-options skipped #50346

Closed

richardlau added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Jun 22, 2024

tools: fix skip detection of test runner output

d8be819

Fix the Python test harness so that it no longer treats the `# skipped` part of the summary at the end of the built-in test runner output as marking the test as skipped.

This comment was marked as outdated.

Sign in to view

richardlau force-pushed the testharnessskip branch from c3777c0 to d8be819 Compare June 22, 2024 04:11

richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 22, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 22, 2024

This comment was marked as outdated.

Sign in to view

aduh95 reviewed Jun 22, 2024

View reviewed changes

benjamingr approved these changes Jun 22, 2024

View reviewed changes

lpinca approved these changes Jun 22, 2024

View reviewed changes

atlowChemi approved these changes Jun 22, 2024

View reviewed changes

MoLow approved these changes Jun 22, 2024

View reviewed changes

richardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 24, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jun 24, 2024

richardlau added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jun 24, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 24, 2024

nodejs-github-bot merged commit 2068c40 into nodejs:main Jun 24, 2024
60 checks passed

richardlau deleted the testharnessskip branch June 24, 2024 13:08

targos mentioned this pull request Jun 25, 2024

v22.4.0 release proposal #53583

Merged

Karliz24 mentioned this pull request Jul 9, 2024

## 2024-07-02, Version 22.4.0 (Current), @targos Karliz24/Marley-#16

Open

marco-ippolito mentioned this pull request Jul 19, 2024

v20.16.0 proposal #53945

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: fix skip detection of test runner output #53545

tools: fix skip detection of test runner output #53545

richardlau commented Jun 22, 2024 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Jun 22, 2024

aduh95 Jun 22, 2024

richardlau Jun 22, 2024 •

edited

Loading

nodejs-github-bot commented Jun 24, 2024

nodejs-github-bot commented Jun 24, 2024

	skip = skip_regex.search(output.output.stdout)
	if skip:
	logger.info(
	'ok %i %s # skip %s' % (self._done, command, skip.group(1)))

	skip_regex = re.compile(r'(?:\d+\.\.\d+\|ok\|not ok).# SKIP\S\s+(.*)', re.IGNORECASE)
	skip_regex = re.compile(r'(?:\d+\.\.\d+\|ok\|not ok)[^\n]# SKIP\S\s+(.*)', re.IGNORECASE)

tools: fix skip detection of test runner output #53545

tools: fix skip detection of test runner output #53545

Conversation

richardlau commented Jun 22, 2024 • edited Loading

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Jun 22, 2024

aduh95 Jun 22, 2024

Choose a reason for hiding this comment

richardlau Jun 22, 2024 • edited Loading

Choose a reason for hiding this comment

nodejs-github-bot commented Jun 24, 2024

nodejs-github-bot commented Jun 24, 2024

richardlau commented Jun 22, 2024 •

edited

Loading

richardlau Jun 22, 2024 •

edited

Loading